Skip to content

fix: type GeneralNameJson.value as string or directory name JSON - #540

Open
cpruijsen wants to merge 6 commits into
PeculiarVentures:masterfrom
cpruijsen:fix/issue-495
Open

cpruijsen wants to merge 6 commits into
PeculiarVentures:masterfrom
cpruijsen:fix/issue-495

Conversation

@cpruijsen

@cpruijsen cpruijsen commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

GeneralName.toJSON() now adds an optional typedValue next to value. value keeps its current type and output, so existing callers do not break.

typedValue is a union keyed by kind that covers all nine CHOICE arms: otherName (OID plus BER hex), rfc822Name, dNSName, x400Address (BER hex), directoryName (attributes), ediPartyName, uniformResourceIdentifier, iPAddress (address, optional mask and raw bytes) and registeredID. It is left out when a name cannot be represented.

This follows the review asking for a compatible change with a complete typed form. The tradeoff is that JSON.stringify output gains a field.

It also fixes ediPartyName parsing, which now reads the matched [5] node.

Fixes #495

Tests

src/GeneralName.spec.ts (npx vitest run src/GeneralName.spec.ts) covers each kind, an otherName built by hand as an OtherName SEQUENCE, ediPartyName with and without nameAssigner, IPv4 and IPv6 with and without a mask, the omitted case, and a TypeScript consumer narrowing on kind. 8 of these 9 tests fail with src/GeneralName.ts reverted to master. The spec also keeps the two otherName encoding tests from #586.

@coveralls

coveralls commented Sep 28, 2026 •

Copy link
Copy Markdown

Coverage Status

coverage: 72.965% (+0.5%) from 72.435% — cpruijsen:fix/issue-495 into PeculiarVentures:master

@donskov
donskov requested a review from microshine September 29, 2026 15:06

@microshine microshine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revise the JSON typing approach before merging. I would like to keep this update compatible with the existing API and provide a complete typed representation.

There are two issues with the current change:

  1. string | RelativeDistinguishedNamesJson does not describe all toJSON() results. For example, iPAddress returns ASN.1 JSON without typesAndValues. The narrowing used by the new consumer test (typeof json.value !== "string") therefore does not identify a directory name; the proposed type would allow reading json.value.typesAndValues.length for an IP address, which would fail at runtime. The other ASN.1 object variants must also be covered.
  2. Widening the existing value type breaks TypeScript compatibility, even for consumers of string variants. For example, this previously valid assignment stops compiling although its runtime value is still a string:
const email: string = new GeneralName({
  type: 1,
  value: "alice@example.com"
}).toJSON().value;

Please consider extending the existing result with an optional typedValue field instead, preserving the current runtime values and declarations of type and value:

interface GeneralNameJson {
  type: number;
  value: string; // Keep the legacy declaration for compatibility.
  typedValue?: GeneralNameTypedValue;
}

The optional field preserves compatibility with existing object literals, stored JSON from earlier versions, and implementations that already return GeneralNameJson:

const json: GeneralNameJson = {
  type: 2,
  value: "example.com"
};

For a valid DNS name, the extended result would be:

{
  "type": 2,
  "value": "example.com",
  "typedValue": {
    "kind": "dNSName",
    "value": "example.com"
  }
}

GeneralNameTypedValue should be a complete discriminated union with its own kind field. These are suggested shapes for the new typedValue field only; retain the full existing value in each case:

Type Suggested typedValue
0 — otherName {"kind":"otherName","oid":"1.2.3.4","valueBerHex":"0c03666f6f"}
1 — rfc822Name {"kind":"rfc822Name","value":"alice@example.com"}
2 — dNSName {"kind":"dNSName","value":"example.com"}
3 — x400Address {"kind":"x400Address","berHex":"a3023000"}
4 — directoryName {"kind":"directoryName","attributes":[{"oid":"2.5.4.3","asn1Type":"UTF8String","value":"Example"}]}
5 — ediPartyName {"kind":"ediPartyName","nameAssigner":"Example","partyName":"Alice"}
6 — uniformResourceIdentifier {"kind":"uniformResourceIdentifier","value":"https://example.com"}
7 — iPAddress {"kind":"iPAddress","address":"127.0.0.1","bytesHex":"7f000001"}
8 — registeredID {"kind":"registeredID","value":"1.2.3.4"}

A few details to account for:

  • otherName payloads depend on the OID, so the general representation should retain the BER of the payload rather than assume a particular decoded shape.
  • For x400Address, retaining the BER of the original GeneralName ASN.1 node is sufficient; a complete X.400 object model need not be introduced here.
  • The directory-name attributes are a convenient projection; retain their OIDs and ASN.1 types, and keep the full legacy representation in value.
  • nameAssigner is optional for ediPartyName. Please also check the schema-parsing path: it currently reads asn1.result.ediPartyName, but that name is not assigned in the schema. Do not produce a typed representation from missing data.
  • For IP addresses, cover IPv4, IPv6, and the address-and-mask form used in NameConstraints, retaining the original bytes.

Consumers could then use:

const typed = name.toJSON().typedValue;

if (typed?.kind === "directoryName") {
  console.log(typed.attributes);
}

if (typed?.kind === "iPAddress") {
  console.log(typed.address);
}

If a correct typed representation cannot be produced, omit typedValue and preserve the existing behavior of toJSON() rather than introduce new exceptions.

This can be treated as a minor addition if the JSON contract permits additional fields. The output of JSON.stringify() will change, so consumers requiring an exact JSON shape need to be considered. Correcting the existing, imprecise value declaration can be reserved for a future major release.

…ring

Keep GeneralNameJson.value declared as string so existing consumers
still compile, and add an optional typedValue discriminated union keyed
by kind covering all nine GeneralName types. typedValue is omitted when
a faithful representation cannot be produced.

Also parse ediPartyName from the matched [5] node: the schema never
assigned asn1.result.ediPartyName, so parsed ediPartyName values were
undefined.
@cpruijsen

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. Reworked along your suggestion in 3ed8d4d:

  • value stays declared as string. The new optional typedValue is a discriminated union on kind that covers all nine types, using the shapes from your table.
  • typedValue is omitted when a faithful representation cannot be produced. toJSON() throws nothing new.
  • iPAddress covers IPv4, IPv6 and the address-and-mask form, and keeps the original bytes.
  • ediPartyName is now parsed from the matched [5] node. asn1.result.ediPartyName was never assigned, so parsed values were undefined before. nameAssigner is optional.
  • Tests cover each kind, the omission case, and your const email: string = ... compatibility example.

Master now only collects src/**/*.spec.ts. Also formats both files with oxfmt and renames a local that shadowed the hex import.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

type of GeneralNameJson is wrong

3 participants